Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(eviction): Tune eviction threshold in cache mode #4142

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

BagritsevichStepan
Copy link
Contributor

@BagritsevichStepan BagritsevichStepan commented Nov 18, 2024

fixes #4139

@BagritsevichStepan BagritsevichStepan changed the title cache(cache_mode): Tune eviction threshold in cache mode fix(cache_mode): Tune eviction threshold in cache mode Nov 18, 2024
@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch from cdd7fc0 to b5e1611 Compare November 18, 2024 10:14
@BagritsevichStepan BagritsevichStepan changed the title fix(cache_mode): Tune eviction threshold in cache mode fix(eviction): Tune eviction threshold in cache mode Nov 18, 2024
@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch 5 times, most recently from 5e6c873 to 2d7dd27 Compare November 25, 2024 10:12
@BagritsevichStepan BagritsevichStepan self-assigned this Nov 25, 2024
@BagritsevichStepan
Copy link
Contributor Author

@adiholden I believe I've identified the issue: I'm calculating rss_threshold for each shard individually, but the current rss_memory value is a global metric for all shards

@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch 3 times, most recently from 9bd58cc to fe22e03 Compare November 26, 2024 18:05
await asyncio.sleep(1) # Wait for RSS heartbeat update

# First test that eviction is not triggered without connection creation
stats_info = await async_client.info("stats")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets also add a check here that used memory is below maxmemory * 0.9 and rss curr memory is below
maxmemory * rss_oom_deny_ratio * 0.9

# Increase RSS memory by 30% of max memory
# We can simulate RSS increase by creating new connections
# Estimate memory per connection
estimated_connection_memory = 15 * 1024 # 15 KB per connection
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this test supper stable and more controlled
I would run
CONFIG SET enable_heartbeat_eviction false
before creating the connections
after that check the rss make sure its above the threshold
then run
CONFIG SET enable_heartbeat_eviction true
and after that check that rss memory dropped below the threshold and check the stats for evicted keys as you did below

@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch from 0097d0a to 40d6063 Compare November 28, 2024 18:18
@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch from 40d6063 to 692a3fa Compare December 2, 2024 09:26
@BagritsevichStepan BagritsevichStepan force-pushed the memory/tune-eviction-threshold-in-cache-mode branch from 692a3fa to 051412e Compare December 2, 2024 09:44
return max_memory - used_memory;
}
// negative value indicates memory overuse
return -1 * ssize_t(used_memory - max_memory);
Copy link
Contributor

@BorysTheDev BorysTheDev Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect results if max_memory > used_memory because size_t - size_t = size_t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ssize_t CalculateMemoryBudget(size_t max_memory, size_t used_memory) {
  if (max_memory >= used_memory) {
    return max_memory - used_memory;
  }
  // negative value indicates memory overuse
  return -1 * ssize_t(used_memory - max_memory);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I got, the result is correct because you use overflow of ssize_t type, but it can be UB I'm not sure. please rewrite

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to round size_t to max ssize_t? Because I already implemented this previously
#4142 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just update ssize_t CalculateMemoryBudget(ssize_t max_memory, ssize_t used_memory)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we are still converting size_t to ssize_t before calling the function. This is even more dangerous because we did not perform the subtraction.

Copy link
Contributor Author

@BagritsevichStepan BagritsevichStepan Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, we are using this values

atomic_uint64_t used_mem_peak(0);
atomic_uint64_t used_mem_current(0);
atomic_uint64_t rss_mem_current(0);
atomic_uint64_t rss_mem_peak(0);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in our case, such converting is absolutely safe, because we will never get such a memory size in the near future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove oom_deny_ratio flag
3 participants